-
Notifications
You must be signed in to change notification settings - Fork 180
[ENH] Allow additional columns if defined for various TSV files #2054
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@yarikoptic Could I trouble you for a review? I suspect most of our European friends are on holiday, you seem active, and I think this should be straightforward. |
@@ -21,6 +21,7 @@ Blood: | |||
whole_blood_radioactivity: | |||
level: optional | |||
level_addendum: required if `WholeBloodAvail` is `true` | |||
additional_columns: allowed_if_defined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mnoergaard @melanieganz Can you confirm whether blood.tsv files are permitted to have unknown columns? The options are allowed
(warn if undefined by BIDS spec or sidecar), allowed_if_defined
(error if undefined by BIDS spec or sidecar), and not_allowed
(error if undefined by BIDS spec).
42bb79a
to
1125fd2
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2054 +/- ##
=======================================
Coverage 82.06% 82.06%
=======================================
Files 17 17
Lines 1533 1533
=======================================
Hits 1258 1258
Misses 275 275 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1125fd2
to
19cf2f4
Compare
Several table definitions were missing an indication of whether additional columns were allowed, which defaults to
not_allowed
. This PR updates the metaschema to require the field to be explicit, and sets its value toallowed_if_defined
for all cases that were missing.cc @melanieganz @CPernet @mnoergaard for your thoughts on the PET cases. This is strictly speaking a relaxation, but IMO if people provide definitions, that's good?
Closes #1993.